Fix: Empty string as template in CustomPlot.init #841
Conversation
Fixed dvc issue #10482 at iterative/dvc#10482
|
Thanks @anunayasri! As long as we are fixing it for the empty string, can we also handle any string that's not a supported template? |
@dberenbaum how do we recognize that? run Vega parser / validator? |
src/dvclive/plots/custom.py
Outdated
| if template is not None and template.strip() == "": | ||
| template = None |
There was a problem hiding this comment.
I think we should just disallow empty string.
There was a problem hiding this comment.
In any case, this template.strip() is unnecessary.
| if template is not None and template.strip() == "": | |
| template = None | |
| if not template: | |
| template = None |
There was a problem hiding this comment.
@skshetry Thanks. This is much cleaner.
Co-authored-by: skshetry <18718008+skshetry@users.noreply.github.com>
|
There is a missing None check in my commit and mypy failing the pipeline. I have correct it. |
tests/plots/test_custom.py
Outdated
| } | ||
|
|
||
|
|
||
| def test_default_template_in_log_custom_plot(tmp_dir): |
There was a problem hiding this comment.
I don't think it's worth it to add this test, has very less return.
We can't test for every combinations.
There was a problem hiding this comment.
Since template = '' caused a cryptic error, I added this test as a guardrail to ensure that any template = None is handled explicitly.
But on a second thought, I think you are correct. This test is for log_plot() which has default value for template. CustomPlot.__init__() doesn't have a default value for template but that shouldn't be test here.
Will rectify.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #841 +/- ##
==========================================
- Coverage 95.47% 95.18% -0.29%
==========================================
Files 57 55 -2
Lines 3889 3904 +15
Branches 353 349 -4
==========================================
+ Hits 3713 3716 +3
- Misses 124 138 +14
+ Partials 52 50 -2 ☔ View full report in Codecov by Sentry. |
🤔 I forgot we don't render these anywhere here, and supporting custom templates would not be easy, so forget my initial suggestion. |
Fixes #10482 (report in dvc repo) at iterative/dvc#10482
❗ I have followed the Contributing to DVCLive guide.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏